-
Notifications
You must be signed in to change notification settings - Fork 31
Refactoring ExpressionValidation
#3735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactoring ExpressionValidation
#3735
Conversation
📝 WalkthroughWalkthroughAdds NestedDataModelLocationProviders and helpers to derive repeating-group contexts; refactors expression validation to per-field collection with a validation collector; introduces FD.useDebouncedAllPaths to enumerate concrete field paths; adjusts a binding-validation capture; and adds tests/fixtures for component lookups and hidden-state handling in repeating groups. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
/publish |
PR release:
|
de7e932
to
d77219c
Compare
ExpressionValidation
ExpressionValidation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ser bra ut!
…kily this made a test fail, but I went down the wrong track when debugging it. The function was always supposed to return the input if nothing else was found. Also, the current value of the field is not really needed as an input for useEffect()
… do not want to run expression validation for a row when there are no rows, so the last solution didn't work as we wanted (it caused lots of errors in the developer tools log)
… schema, just data
2f896e7
to
7a64e36
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/features/expressions/shared-functions.test.tsx
(3 hunks)src/features/formData/FormDataWrite.tsx
(2 hunks)src/features/saveToGroup/layoutValidation.ts
(1 hunks)src/features/validation/expressionValidation/ExpressionValidation.tsx
(2 hunks)src/features/validation/expressionValidation/shared-expression-validation-tests/component-lookup-hidden.json
(1 hunks)src/utils/layout/DataModelLocation.tsx
(2 hunks)test/e2e/integration/expression-validation-test/expression-validation.ts
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}
: Avoid usingany
and unnecessary type casts (as Type
) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/features/formData/FormDataWrite.tsx
src/utils/layout/DataModelLocation.tsx
src/features/saveToGroup/layoutValidation.ts
test/e2e/integration/expression-validation-test/expression-validation.ts
src/features/validation/expressionValidation/ExpressionValidation.tsx
src/features/expressions/shared-functions.test.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProviders
fromsrc/test/renderWithProviders.tsx
to supply required form layout context
Files:
src/features/expressions/shared-functions.test.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-22T13:53:28.252Z
Learnt from: CR
PR: Altinn/app-frontend-react#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-22T13:53:28.252Z
Learning: Applies to **/*.test.{ts,tsx} : In tests, use `renderWithProviders` from `src/test/renderWithProviders.tsx` to supply required form layout context
Applied to files:
src/features/expressions/shared-functions.test.tsx
🧬 Code graph analysis (5)
src/features/formData/FormDataWrite.tsx (1)
src/features/datamodel/DataModelsProvider.tsx (1)
DataModels
(380-421)
src/features/saveToGroup/layoutValidation.ts (1)
src/utils/layout/generator/validation/hooks.ts (1)
validateDataModelBindingsSimple
(58-77)
test/e2e/integration/expression-validation-test/expression-validation.ts (2)
src/features/validation/index.ts (1)
IExpressionValidationConfig
(263-266)src/features/expressions/types.ts (1)
Expression
(71-71)
src/features/validation/expressionValidation/ExpressionValidation.tsx (5)
src/features/validation/index.ts (2)
FieldValidations
(92-94)IExpressionValidation
(222-227)src/features/datamodel/DataModelsProvider.tsx (1)
DataModels
(380-421)src/utils/layout/DataModelLocation.tsx (1)
NestedDataModelLocationProviders
(170-188)src/utils/layout/useExpressionDataSources.ts (2)
useExpressionDataSources
(104-167)ExpressionDataSources
(37-55)src/features/expressions/index.ts (1)
evalExpr
(70-118)
src/features/expressions/shared-functions.test.tsx (5)
src/features/form/layout/LayoutsContext.tsx (1)
useLayoutLookups
(90-90)src/features/expressions/expression-functions.ts (1)
component
(427-463)src/layout/layout.ts (1)
IDataModelBindings
(61-64)src/features/form/layout/utils/repeating.ts (2)
RepeatingComponents
(5-5)getRepeatingBinding
(21-37)src/utils/layout/DataModelLocation.tsx (1)
NestedDataModelLocationProviders
(170-188)
🪛 Biome (2.1.2)
src/utils/layout/DataModelLocation.tsx
[error] 179-182: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Install
- GitHub Check: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (1)
src/features/saveToGroup/layoutValidation.ts (1)
56-56
: LGTM! Correctness fix for return type handling.The change correctly removes the array destructuring since
validateDataModelBindingsSimple
returnsstring[]
directly, not a tuple. The previous destructuring would have incorrectly extracted the first string element instead of the array itself.
src/features/validation/expressionValidation/ExpressionValidation.tsx
Outdated
Show resolved
Hide resolved
test/e2e/integration/expression-validation-test/expression-validation.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/features/formData/FormDataWrite.tsx (1)
675-702
: Fix TypeScript indexing ofunknown
data in path collector.Lines 688 and 692 perform element access on
data
which is typed asunknown
. Even with the type guard on line 688, TypeScript still treatsdata
asunknown
afterward. Under strict TypeScript configurations (such asnoUncheckedIndexedAccess
or similar), indexingunknown
is a compile error.Apply this diff to add a safe cast after the type guard:
function collectMatchingFieldPaths( data: unknown, fieldParts: string[], currentPath: string, partIndex: number, results: string[], ) { if (partIndex >= fieldParts.length) { results.push(currentPath); return; } const part = fieldParts[partIndex]; - if (typeof data !== 'object' || data === null || data[part] === undefined || data[part] === null) { + if (typeof data !== 'object' || data === null) { return; } - const nextData = data[part]; + const container = data as Record<string, unknown>; + const nextData = container[part]; + if (nextData === undefined || nextData === null) { + return; + } + const nextPath = currentPath ? `${currentPath}.${part}` : part; if (Array.isArray(nextData)) { for (let i = 0; i < nextData.length; i++) { collectMatchingFieldPaths(nextData[i], fieldParts, `${nextPath}[${i}]`, partIndex + 1, results); } } else { collectMatchingFieldPaths(nextData, fieldParts, nextPath, partIndex + 1, results); } }src/utils/layout/DataModelLocation.tsx (1)
177-188
: Fix Biome lint error by replacing reduceRight with a reverse loop.Biome flags
groupContexts.reduceRight
forlint/correctness/useJsxKeyInIterable
because JSX is returned from an iterable callback. Even though a key is provided, Biome requires a different pattern to satisfy this rule.Apply this diff to replace the reduceRight with a reverse for-loop:
- // Recursively nest the providers from outermost to innermost - return groupContexts.reduceRight( - (child, { groupBinding, rowIndex }) => ( - <DataModelLocationProvider - key={`${groupBinding.dataType}-${groupBinding.field}-${rowIndex}`} - groupBinding={groupBinding} - rowIndex={rowIndex} - > - {child} - </DataModelLocationProvider> - ), - children as React.ReactElement, - ); + // Nest the providers from outermost to innermost + let wrappedChildren: React.ReactNode = children; + for (let i = groupContexts.length - 1; i >= 0; i--) { + const { groupBinding, rowIndex } = groupContexts[i]; + wrappedChildren = ( + <DataModelLocationProvider + key={`${groupBinding.field}-${rowIndex}`} + groupBinding={groupBinding} + rowIndex={rowIndex} + > + {wrappedChildren} + </DataModelLocationProvider> + ); + } + return <>{wrappedChildren}</>;
🧹 Nitpick comments (1)
src/features/formData/FormDataWrite.tsx (1)
816-848
: Simplify the lookup condition logic.The condition on lines 832-833 is somewhat convoluted. Since
foundInDataModel
already checkslookupTool && (!lookupErr || lookupErr.error !== 'missingProperty')
, the subsequent check forlookupErr?.error !== 'missingRepeatingGroup'
can be simplified.Additionally, line 834 uses
reference?.field
with optional chaining, but we've already confirmedreference
is defined on line 826.Consider this refactor for clarity:
return useShallowSelector((v) => { if (!reference) { return emptyArray; } - // When lookupTool is available and doesn't report a missing repeating group error, we know there's no - // repeating group structure in this path, so we can return the field as-is. - const foundInDataModel = lookupTool && (!lookupErr || lookupErr.error !== 'missingProperty'); - if (foundInDataModel && lookupErr?.error !== 'missingRepeatingGroup') { - return [reference?.field]; + // When the field exists in the data model and is not part of a repeating group, + // we can return the field as-is without enumeration. + if (lookupTool && lookupErr?.error !== 'missingRepeatingGroup' && lookupErr?.error !== 'missingProperty') { + return [reference.field]; } // If lookupTool is not available (e.g., in tests), or if there's a missingRepeatingGroup error, // we need to check the actual data to find all matching paths. const formData = v.dataModels[reference.dataType]?.debouncedCurrentData; if (!formData) { return []; } const paths: string[] = []; collectMatchingFieldPaths(formData, reference.field.split('.'), '', 0, paths); return paths.length === 0 ? [reference.field] : paths.sort(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/features/formData/FormDataWrite.tsx
(2 hunks)src/features/validation/expressionValidation/ExpressionValidation.tsx
(2 hunks)src/utils/layout/DataModelLocation.tsx
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}
: Avoid usingany
and unnecessary type casts (as Type
) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/features/formData/FormDataWrite.tsx
src/features/validation/expressionValidation/ExpressionValidation.tsx
src/utils/layout/DataModelLocation.tsx
🧬 Code graph analysis (2)
src/features/formData/FormDataWrite.tsx (1)
src/features/datamodel/DataModelsProvider.tsx (1)
DataModels
(380-421)
src/features/validation/expressionValidation/ExpressionValidation.tsx (5)
src/features/validation/index.ts (2)
FieldValidations
(92-94)IExpressionValidation
(222-227)src/features/datamodel/DataModelsProvider.tsx (1)
DataModels
(380-421)src/features/validation/validationContext.tsx (1)
Validation
(320-375)src/utils/layout/DataModelLocation.tsx (1)
NestedDataModelLocationProviders
(170-189)src/utils/layout/useExpressionDataSources.ts (2)
useExpressionDataSources
(104-167)ExpressionDataSources
(37-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (8)
src/features/validation/expressionValidation/ExpressionValidation.tsx (5)
43-51
: LGTM - Collector memoization prevents infinite loop.The collector object is now properly memoized with
useMemo
and an empty dependency array, which prevents the infinite render loop flagged in the previous review. While the implementation differs slightly from the suggested fix (inline function vs. separateuseCallback
), it achieves the same stability guarantee since React'ssetState
functions are stable anduseMemo
with empty deps only runs once.
53-59
: LGTM - Effect properly updates validation state.The effect correctly updates data model validations when
allFieldValidations
changes, with appropriate guards for missingdataElementId
.
61-78
: LGTM - Rendering logic is clean and correct.The early returns prevent rendering when necessary data is unavailable, and each field validation is properly keyed and receives the correct props.
80-110
: LGTM - Per-field path enumeration properly handled.This component correctly uses
FD.useDebouncedAllPaths
to enumerate all concrete field paths (handling repeating groups), then wraps each path inNestedDataModelLocationProviders
to establish the proper context for validation. This aligns with the PR objective of fixing component lookups inside repeating groups.
112-164
: LGTM - Validation computation is correct and efficient.The component properly:
- Sets up expression data sources with the correct context
- Evaluates validation conditions and messages using
evalExpr
- Collects validation results and reports them to the collector
- Uses appropriate memoization (dataSources via useMemo) to prevent unnecessary effect re-runs
src/utils/layout/DataModelLocation.tsx (3)
25-28
: LGTM - Interface is clear and well-typed.The
LocationProps
interface properly defines the props forDataModelLocationProvider
.
30-30
: LGTM - Cleaner type definition.Using
PropsWithChildren<LocationProps>
is cleaner and more maintainable than an inline anonymous type.
122-161
: LGTM - Array index extraction is correct.The
parseGroupContexts
function properly parses field paths to extract array indexes and build group contexts. The logic correctly handles:
- Multiple array indexes in a single path
- Building incremental paths for nested structures
- Edge cases like empty fields or no array indexes
|
Description
Refactoring to fix issues with
component
lookups inside repeating groups by usingDataModelLocationProvider
properly. This used to look up expression data sources for everything at once, thus missing the nuances of different rows in repeating groups.Related Issue(s)
component
#3741Verification/QA
kind/*
andbackport*
label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Bug Fixes
Tests